-
Notifications
You must be signed in to change notification settings - Fork 18
feat: Dashboard Redesign #160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Hi, @ohansFavour, just out of curiosity, is there already a rough estimate when this change will get published? |
Provider changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
src/api/analytics/index.ts
Outdated
| export const useAnalyticsService = () => { | ||
| const fetchData = useFetchData(); | ||
|
|
||
| const fireEvent = async (data: Record<string, unknown>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be inside a useCallback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| try { | ||
| let email: string | null = "[email protected]"; | ||
|
|
||
| if (getAuthMode() === "email-password") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we do this for TP logins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authMode represents the authentication method used to sign into the dashboard. We currently support two modes: api-key and email-password
| <Box | ||
| key={provider.id} | ||
| className="add-new-provider-modal__providers__list__item"> | ||
| <Button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the action defined for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ignore. Deleted file
|
|
||
| import { useRolesList } from "../hooks"; | ||
|
|
||
| interface CreateNewRoleModalProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we didn't use interfaces at all
| interface CreateNewRoleModalProps { | |
| type CreateNewRoleModalProps = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
src/features/users/hooks/useUsers.ts
Outdated
| void queryClient.invalidateQueries({ predicate: (query) => query.queryKey[0] === QUERY_KEYS.USERS_SEARCH }); | ||
| void queryClient.invalidateQueries({ | ||
| predicate: (query) => query.queryKey[0] === QUERY_KEYS.USERS_INFINITE, | ||
| }); | ||
| void queryClient.invalidateQueries({ predicate: (query) => query.queryKey[0] === QUERY_KEYS.USERS_COUNT }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we include the tenant ID in the query key instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| const toggleNull = () => { | ||
| if (currentValue === null) { | ||
| // Restore to default value or appropriate zero value | ||
| if (config.valueType === "number") { | ||
| setCurrentValue(config.defaultValue !== null ? config.defaultValue : 0); | ||
| } else if (config.valueType === "boolean") { | ||
| setCurrentValue(config.defaultValue !== null ? config.defaultValue : false); | ||
| } else { | ||
| setCurrentValue(config.defaultValue !== null ? config.defaultValue : ""); | ||
| } | ||
| } else { | ||
| setCurrentValue(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a useCallback w/ setCurrentValue(currentValue => ...
| } | ||
| }; | ||
|
|
||
| const renderValueInput = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this extracted like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a personal preference. Extracting this logic makes the main component's JSX cleaner and easier to read, with the complex input rendering logic abstracted away.
| } | ||
| }; | ||
|
|
||
| const handleSaveProperty = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useCallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion. In this case, handleSaveProperty isn’t passed to any memoized child or used in a dependency array, so wrapping it in useCallback wouldn’t provide a measurable benefit. Since it’s only used locally within this component, keeping it as a regular function keeps the code simpler and easier to read. If we later memoize Button or reuse this handler, I’ll revisit adding useCallback.
| </Flex> | ||
| </Flex> | ||
|
|
||
| {isEditModalOpen && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these rendered for every row?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. I have moved these to the table component
| databaseType: "postgres" | "mysql" | null; | ||
| } | ||
|
|
||
| export default function PluginPropertiesSection({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer it if PluginProperties* was renamed to DatabaseProperties
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [tenantId, providerId, isAddingNewProvider, initialProviderConfigResponse]); | ||
|
|
||
| const handleThirdPartyIdSuffixChange = (e: React.ChangeEvent<HTMLInputElement>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rework these to handlers to use callbacks passed to the state setter in order to avoid dependencies on the current value and use useCallback to memoize them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated ✅
| errors: ValidationErrors | ||
| ): void => { | ||
| // Validate client ID | ||
| if (typeof client.clientId !== "string" || client.clientId.trim() === "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not reuse the above validators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated ✅
| await onDeleteTenant(); | ||
| goToTenantsList(); | ||
| } catch (err) { | ||
| // Error handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated ✅
src/features/tenants/Page.tsx
Outdated
| const [searchParams] = useSearchParams(); | ||
| const tenantId = searchParams.get(QUERY_PARAMS.TENANT_ID); | ||
|
|
||
| return tenantId ? <TenantDetails /> : <TenantsList />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we pass tenantId as a prop if we get it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated ✅
| const [openDeleteModal, setOpenDeleteModal] = useState(false); | ||
| const [openUnlinkModal, setOpenUnlinkModal] = useState(false); | ||
|
|
||
| // VERIFY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets discuss then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation (status quo) uses
user.isPrimaryUser && user.loginMethods.length === 1. This allows unlinking the last login method for primary users. This seems problematic
| export const setSelectedTenantIdToLocalStorage = (tenantId: string) => { | ||
| localStorageHandler.setItem(StorageKeys.TENANT_ID, tenantId); | ||
| }; | ||
|
|
||
| export const getSelectedTenantIdFromLocalStorage = (): string | undefined => { | ||
| return localStorageHandler.getItem(StorageKeys.TENANT_ID); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd much prefer if we did this based on the path or some queryparam
src/shared/version.ts
Outdated
| * under the License. | ||
| */ | ||
|
|
||
| export const package_version = "0.13.0"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be updated.
| @@ -47,7 +47,7 @@ export const ThirdPartyPage = ({ | |||
| <span>Back to tenant info</span> | |||
| </button> | |||
| <div className="third-party-section__cards"> | |||
| <TenantDetailHeader onlyShowTenantId /> | |||
| {/* <TenantDetailHeader onlyShowTenantId /> */} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please either remove it completely (incl the comment) or not, but don't leave it as a comment.
src/utils/generateRandomId.ts
Outdated
| * under the License. | ||
| */ | ||
|
|
||
| export const generateRandomId = (): number => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these files duplicated?
| }, [availableTags]); | ||
|
|
||
| // Trigger search when search entries change | ||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this triggered using a useEffect?
Summary of change
This change implements the new user management dashboard design
Figma: https://www.figma.com/design/9mtSPHJC5722JKymuEj4uU/Dashboard-Redesign
Problem Statement
The old dashboard's design is not consistent with SuperTokens' design system. The new dashboard improves both the UI and the UX of the old dashboard.
Summary of solution
Most of the changes are related to the styling of components and pages. The new dashboard uses radix UI component library https://www.radix-ui.com/
Test Plan
Tested on all primary browsers for:
Feature tests:
Dashboard Admin access.
POST,PUTandDELETEendpoints with admins only access enabled for the dashboard recipe.POST,PUTandDELETEendpoints without the admins only access enabled.Search
General UI testing
Multi tenant testing
User Roles and Permissions testing
feature_not_enabledstate on both userDetails page and user roles page.User creation
emailpasswordandthirdpartyemailpasswordtogether and individually.passwordlessandthirdpartypasswordlesstogether and individually.contactMethod's ensure that the frontend displays relevant UI based on thecontactMethodselected.emailpasswordandpasswordlessuser with the same email and make sure that the accounts are linked.User details
Tenant Management
recipe,config,hello,appid-t1Sometimesoption is selected forHow often does the provider return email?Alwaysoption is selected forHow often does the provider return email?Neveroption is selected forHow often does the provider return email?Sometimesand the fake email generation is set to trueDocumentation changes
(If relevant, please create a PR in our docs repo, or create a checklist here highlighting the necessary changes)
Checklist for important updates
package.jsonpackage-lock.jsonsrc/version.tsnpm run buildRemaining TODOs for this PR